Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: input control drag and box control change #25933

Merged
merged 4 commits into from
Oct 14, 2020
Merged

Fix: input control drag and box control change #25933

merged 4 commits into from
Oct 14, 2020

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Oct 8, 2020 •
edited
Loading

As an alternative to #25913, adds some changes that fix the issues:

  • avoid overwriting the react-use-gesture drag handlers 54e0484
  • fix InputControl's handling of blur commits 78a0e90

Additionally, some minor enhancements:

  • let UnitControl trigger parsed unit changes even if the parsed value is the same 79949f9
  • corrects a small oversight in NumberControl to get value resolution working for blur commits d6aec80

Here's visual of what the UnitControl change enables:

Before After
unit-change-plz unit-change-haz

How has this been tested?

Storybook and Wordpress 5.5.1

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@stokesman stokesman marked this pull request as draft October 8, 2020 07:05
@ItsJonQ
Copy link

ItsJonQ commented Oct 8, 2020

@stokesman Haii! Just a heads up, the BoxControl components (using UnitControls) need isPressEnterToChange. The reason is because it allows for the auto unit parsing step.

That is... typing in 50%, it can parse that out as [50, %], and propagate those changes.

@stokesman stokesman force-pushed the fix/input-control-drag-and-box-control-change branch from 427d1fa to 79949f9 Compare October 9, 2020 19:26
@stokesman stokesman mentioned this pull request Oct 9, 2020
Closed
1 task
@stokesman stokesman marked this pull request as ready for review October 9, 2020 22:32
stokesman
stokesman commented Oct 9, 2020
View reviewed changes
Copy link
Contributor Author

@stokesman stokesman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e801eed803d1b49be53d5b8b488db988c32516e3 This diff is kind of funky but(rebased/edited it and the diff is easier to read) I think the overall flow of the code is easier to understand now. The gist of it is to handle the parse of any entered value+unit sooner than was previously done (by the state reducer) and update state / propagate at the same time. Then all the state reducer has to do is pass it along.

@stokesman stokesman force-pushed the fix/input-control-drag-and-box-control-change branch from 79949f9 to 575ea9b Compare October 12, 2020 06:06
@stokesman

This comment has been minimized.

Sign in to view
@stokesman stokesman marked this pull request as draft October 12, 2020 15:08
@stokesman stokesman force-pushed the fix/input-control-drag-and-box-control-change branch from 575ea9b to d6aec80 Compare October 12, 2020 16:30
@stokesman stokesman marked this pull request as ready for review October 12, 2020 18:01
@ItsJonQ
Copy link

ItsJonQ commented Oct 13, 2020

@stokesman Haiii! I just tested this out on my end. It looks like it's working very well!
Tested it across Chrome, Safari, Firefox. Local Gutenberg and Storybook.

Tested drag interactions, "reset" interactions, intentionally going beyond min/max values via keyboard, and up/down arrows.

I left a comment for something to resolve. Once that's done, I feel okay merging this in, and closing the PR I had:
#25913

Note: I hope the the state handling/flow of these components can be improved in the future 🙏 .
That being said, I don't think the use cases would become simpler, as this logic is needed to handle complex interactions.

@stokesman stokesman force-pushed the fix/input-control-drag-and-box-control-change branch from d6aec80 to a326f56 Compare October 13, 2020 20:46
@stokesman
Copy link
Contributor Author

stokesman commented Oct 13, 2020 •
edited
Loading

@ItsJonQ, thanks for testing and reviewing!

That being said, I don't think the use cases would become simpler, as this logic is needed to handle complex interactions.

I'm not sure if you're talking about the UnitControl changes or the general idea of using the focus status as part of state handling logic.

If you'd prefer, I'd be glad to drop that UnitControl change (09a4127495dc66eea57b9e3a196e489c4d4fb35b) from this PR and make it into its own. I would think the old flow could be retained but still enable the new feature (changing units from a parse even when the value didn't change).

@ItsJonQ
Copy link

ItsJonQ commented Oct 13, 2020

I'm not sure if you're talking about the UnitControl changes or the general idea of using the focus status as part of state handling logic.

@stokesman Oh, I'm talking about InputControl based UI in general (not of your specific change). Apologies for lack of clarity!

I think your UnitControl fix is great! Thank you for that 🙏

@ItsJonQ ItsJonQ modified the milestones: 5.6, Gutenberg 9.2 Oct 13, 2020
@ItsJonQ
Copy link

ItsJonQ commented Oct 14, 2020

Hm... I'm really not sure what the PHP unit test may be failing.
@stokesman Would you be able to rebase/merge in the latest master, just in case?

Once tests are green, then I'd be happy to merge this PR :)

Thanks!

@stokesman
avoid overwriting drag gesture handler in InputControl
ebd9c98
@stokesman
revise InputControl state handling for commits on blur
0b4eac1 
- adds wasDirtyOnBlur and its use in syncing logic
- removes a bit of lint
@stokesman
improve unit changing logic in UnitControl
b6cad79 
allows unit to change on parses even if parsed value is unchanged
@stokesman
correct an oversight in NumberControl
3f788c4
@stokesman stokesman force-pushed the fix/input-control-drag-and-box-control-change branch from a326f56 to 3f788c4 Compare October 14, 2020 16:39
@stokesman
Copy link
Contributor Author

@ItsJonQ, rebased on master and 💚

ItsJonQ
ItsJonQ approved these changes Oct 14, 2020
View reviewed changes
Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 from me! Thanks @stokesman for this update 🙏

@ItsJonQ ItsJonQ merged commit 160464a into WordPress:master Oct 14, 2020
@stokesman stokesman mentioned this pull request Mar 29, 2021
Open
@ciampo ciampo mentioned this pull request Mar 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItsJonQ ItsJonQ ItsJonQ approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Gutenberg 9.2
Development

Successfully merging this pull request may close these issues.
2 participants
@stokesman @ItsJonQ